Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: enable big toc for release builds in AIX #7508

Closed
wants to merge 1 commit into from
Closed

build: enable big toc for release builds in AIX #7508

wants to merge 1 commit into from

Conversation

gireeshpunathil
Copy link
Member

@gireeshpunathil gireeshpunathil commented Jul 1, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

NOTE: There is an outstanding issue in AIX build (#7500) - build fail due to 787eddf . The tests are conducted with this change removed.

Affected core subsystem(s)
Description of change

Issue #7500

AIX linker has a table of contents with default size 64K
The recent code inclusions in V8 brings in lot of new
symbols which necessitates to increase this default.

Please note that the debug build already has this flag

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Jul 1, 2016
@bnoordhuis
Copy link
Member

Does that mean the -Wl,-bbigtoc,-bE:<(PRODUCT_DIR)/node.exp in node.gyp can be removed now or is that still necessary?

@gireeshpunathil
Copy link
Member Author

@bnoordhuis - I will check and get back on this.

AIX linker has a table of contents with default size 64K
The recent code inclusions in V8 brings in lot of new
symbols which necessitates to increase this default.

Please note that the debug build already has this flag
@gireeshpunathil
Copy link
Member Author

@bnoordhuis - you are right, the -bbigtoc flag in node.gyp is meant for the same purpose, but covers only the building of node. Now that the same is introduced in common.gypi, we don't need it in node.gyp any more. Please note however, that the -bE:<(PRODUCT_DIR)/node.exp needs to be retained.

And hence I have revised my changes. Please review and let me know.

@bnoordhuis
Copy link
Member

@mhdawson
Copy link
Member

mhdawson commented Jul 5, 2016

LGTM

@mhdawson mhdawson self-assigned this Jul 5, 2016
@mhdawson
Copy link
Member

mhdawson commented Jul 5, 2016

CI run green, landing

mhdawson pushed a commit that referenced this pull request Jul 5, 2016
AIX linker has a table of contents with default size 64K
The recent code inclusions in V8 brings in lot of new
symbols which necessitates to increase this default.

Please note that the debug build already has this flag

Fixes: #7500
PR-URL: #7508
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@mhdawson
Copy link
Member

mhdawson commented Jul 5, 2016

d80432d

@mhdawson
Copy link
Member

mhdawson commented Jul 5, 2016

AIX CI run to validate after landing: https://ci.nodejs.org/job/node-test-commit-aix/242/. We have identified that there is a new issue, but want to validate through CI that the toc issue is resolved.

@mhdawson
Copy link
Member

mhdawson commented Jul 5, 2016

Ok, toc issue resolve, and see new issue, will open separate issue for that one closing

@mhdawson mhdawson closed this Jul 5, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 6, 2016
AIX linker has a table of contents with default size 64K
The recent code inclusions in V8 brings in lot of new
symbols which necessitates to increase this default.

Please note that the debug build already has this flag

Fixes: #7500
PR-URL: #7508
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123 Fishrock123 mentioned this pull request Jul 6, 2016
@MylesBorins
Copy link
Contributor

@mhdawson lts?

@MylesBorins
Copy link
Contributor

needs to land if #6734 lands

@mhdawson
Copy link
Member

Id say if needed due to other commits, yes, otherwise if it applies cleanly then it probably makes sense just to avoid problems in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants